-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ユーザー一覧の非Vue化をする #7426
ユーザー一覧の非Vue化をする #7426
Conversation
094ad67
to
2b5da72
Compare
fab4392
to
232c282
Compare
@machida さん このPRで、デザインの調整は不要なのですが一点確認いただけますと嬉しいです🙇♂️ 確認して欲しいことフォローが下記の状態ですが、大丈夫そうかチェックをいただきたいです🙏 現状先ほどの画像は ですので、大丈夫だと思いますが念のためご確認をお願いいたします。 |
おお、なんだこれ? |
@yocchan-git ちょこっと手を入れて潜らないようにしましたー |
e6bc7f2
to
9c73471
Compare
machida さん修正ありがとうございました! @kyokucho1989 さん 変更確認のチェックを使っていただいても、他でも構いませんので |
@yocchan-git |
@yocchan-git この機能って他の人の作業と被る場合がありましたっけ? いつかのMTG中にそんな話があったかと思いましたが、忘れてしまって… ほかの動作確認は少しずつやっています。もうしばらくお待ちください…! |
早い対応ありがとうございます! (参考)対象のPRです
はい!お時間のある時によろしくお願いいたします🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yocchan-git
遅くなりました。レビューしました。
user.vue
と重複する箇所が多く、今後user.vue
を非Vue化しようとしたときにコードを追跡することが難しくなりそうです..
user.vue
を非Vue化できないか、またはuser.vueを app/views/users/_user_list.html.slim
に埋め込んで利用できないかなどを考える必要があるかと思います。
レビューいただきありがとうございます! 変更が膨大なのと、コードを書いてないとわからないですよね...
こちら、先ほども説明しましたが、
上記の通り、もうしています!になります✌️ こちらを踏まえて、もう一度レビューしていただけますと嬉しいです🙇♂️ |
@yocchan-git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yocchan-git
お疲れ様です!user.vue
は他で使っているため削除できず、Slim形式でファイルを追加している件は了解しました。
他、コードで気になった点をコメントしてます。
27c860f
to
c4b366c
Compare
やっとテストが通りました! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yocchan-git
Approveしますー。長い期間、ありがとうございました!
kyokucho1989 さん @komagata さん |
aa6224b
to
2f5ad40
Compare
7378029
to
1163674
Compare
c39dae5
to
892701c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
相談部屋のコードtalks
の部分は検索関連で処理をSearchUser
クラスに共通化しましたので
ロジックを修正しています。
動作とテストが通ることなどは確認しましたので、コードも見ていただければと思います!
@komagata さん 指摘いただいた点、修正しました! お手隙でレビューよろしくお願いいたします |
app/models/search_user.rb
Outdated
class SearchUser | ||
attr_reader :search_word | ||
|
||
def initialize(search_word:, users: nil, target: nil, require_retire_user: false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constructorではエラーがおきるような、複雑な処理をするべきではないです。
変数を設定・初期化するような処理だけにとどめた方がいいです。
newすらできないクラスはテストも非常に難しくなります。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constructorではエラーがおきるような、複雑な処理をするべきではないです。
変数を設定・初期化するような処理だけにとどめた方がいいです。
教えていただきありがとうございます!修正しました🫡
else | ||
@talks = @talks.page(params[:page]) | ||
search_user = SearchUser.new(search_word: params[:search_word], require_retire_user: true) | ||
@search_word = search_user.search_word |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このクラスを使う方から見ると、SearchUser.newに渡したparams[:search_word]
はsearch_user.search_word
と全く同じものだと判断すると思います。(名前が全く同じなので)
実際にはvalidate_search_word
の処理が行われているので違いがでます。勘違いが発生しやすいAPIになっているので別の物にした方がいいと思います。
app/models/search_user.rb
Outdated
# frozen_string_literal: true | ||
|
||
class SearchUser | ||
attr_reader :search_word |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SearchUserクラスのプロパティなのでword
だけでsearch_wordであることは自明だと思います。
プロパティ表記としてもSearchUser#search_word
となり、冗長な名前ではないかとおもいます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
プロパティ表記としてもSearchUser#search_wordとなり、冗長な名前ではないかとおもいます。
おっしゃる通り、冗長でした...
こちらも修正しましたので、ご確認お願いいたします🙇♂️
@komagata さん 指摘いただいた点修正しました! |
test/models/search_user_test.rb
Outdated
require 'test_helper' | ||
|
||
class SearchUserTest < ActiveSupport::TestCase | ||
test 'ユーザーを指定しない場合' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このプロジェクトの他のテストに合わせてテスト名は英語でお願いします。
一部日本語のテスト名がありますが、それはレビューをもれてしまっているもので、最終的には修正したいものになっています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@komagata さん
修正しました!お手隙でご確認お願いいたします
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@komagata さん
お手数おかけして申し訳ございません!
修正しましたので、ご確認いただけますと嬉しいです🙇♂️
49cdceb
to
467a3b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確認させていただきました。OKです~👌
Issue
概要
ユーザ一覧画面について非Vue化を進めた
変更した箇所
元からRailsで手をつけていないユーザ一覧
変更確認方法
feature/convert-user-index-vue-to-slim
をローカルに取り込むforeman start -f Procfile.dev
でサーバーを立ち上げ、komagata
, password:testtest
)ログインするScreenshot
変更前
変更後